-
Notifications
You must be signed in to change notification settings - Fork 2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Split lib/products-values
#44567
Split lib/products-values
#44567
Conversation
Here is how your PR affects size of JS and CSS bundles shipped to the user's browser: Webpack Runtime (~30 bytes removed 📉 [gzipped])
Webpack runtime for loading modules. It is included in the HTML page as an inline script. Is downloaded and parsed every time the app is loaded. App Entrypoints (~3500 bytes removed 📉 [gzipped])
Common code that is always downloaded and parsed every time the app is loaded, no matter which route is used. Sections (~124312 bytes added 📈 [gzipped])
Sections contain code specific for a given set of routes. Is downloaded and parsed only when a particular route is navigated to. Async-loaded Components (~14409 bytes added 📈 [gzipped])
React components that are loaded lazily, when a certain part of UI is displayed for the first time. Legend What is parsed and gzip size?Parsed Size: Uncompressed size of the JS and CSS files. This much code needs to be parsed and stored in memory. Generated by performance advisor bot at iscalypsofastyet.com. |
Looks like I was wrong, there are immediate benefits, actually! 🎉 This PR appears to remove ~14KB (3.5KB gzipped) from the critical path, offloading it to shared chunks that get used in just the sections that require the code. Please disregard the matticbot totals; ICFY calculates totals incorrectly, and makes it seem like this PR disproportionately increases sections to make up for the entrypoint. That's not the case; most of the changes are ending up in shared chunks, which get counted multiple times in the total. For most user journeys, this PR will be a net win. |
Thank you for the review, @scinos! 🙇 |
This Pull Request is now available for translation here: https://translate.wordpress.com/deliverables/4247512 Hi @sgomes, could you please edit the description of this PR and add a screenshot for our translators? Ideally it'd include all of the following strings:
Thank you in advance! |
@a8ci18n: there should be no new strings with this PR. |
Translation for this Pull Request has now been finished. |
lib/products-values
is currently a large monolith, consisting of many different imports and exports. This makes it hard for the bundler to tree-shake effectively, and introduces potential problems because of circular dependencies betweenlib/products-values
andlib/plans
, for example.This PR splits the library into many small modules, while re-exporting everything from the index file. This should result in minimal changes in consumers of this library, while enabling better tree-shaking.
I don't expect immediate performance benefits from this PR, as there are likely other large libraries to untangle first.I was wrong; see the comments below.This PR is working towards the end goal of removing the dependency on lib/plans from the critical path, which may yet turn out to be impossible.
Changes proposed in this Pull Request
index.js
into individual modulesassertValidProduct
into a newutils/
folderconstants.js
to a newtranslations.js
package.json
withsideEffects
config, to enable the bundler to take advantage of the modularisation of the librarylib/plans
andlib/domains
refer to the new individual modules instead of the index file, to avoid circular dependenciesTesting instructions
This PR doesn't change any functional code, it only moves some code around. As such, a correct build and passing automated tests should be enough to ensure that nothing got broken.